Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[review wanted] add support for locally installed typehints #1895

Closed
wants to merge 5 commits into from

Conversation

tharvik
Copy link
Contributor

@tharvik tharvik commented Jul 18, 2016

Implement example given in PEP 484, in chapter on how to store and distribute stub files, this is functional but not really nice, would be better to implement it via pkg_resources at module loading time.

Prefer locally installed stubs (--user) rather than system wide for easier user customization. Also, in general, one of the most important stubs location, more than typeshed, as it might be more up to date than it.

@gvanrossum
Copy link
Member

I like this idea, but I think it needs some more comments explaining what's happening. I also suspect that the paths where to look are actually platform-dependent (I doubt that on Windows looking in %HOME%\.local\shared\typehints is the right place, nor \usr\shared\typehints.)

@gvanrossum
Copy link
Member

How do you plan to address these questions?

@tharvik
Copy link
Contributor Author

tharvik commented Jul 22, 2016

I've just added some comments and a system independant way to find the data dir.

@tharvik
Copy link
Contributor Author

tharvik commented Jul 22, 2016

CI is failling because of missing site module in typeshed, here it goes typeshed#389.

@gvanrossum
Copy link
Member

@JukkaL: Can you have a look at this? This seems an innocuous PR but it establishes a new place where type hints might be installed.

@tharvik: I think this needs an update to the docs with examples of how a package can install its own stubs. (And you should test all this?)

Also, maybe default_data_dir() could benefit from some of the constants used here (site.USER_BASE, sys.prefix)?

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 22, 2016

I like the idea. Should we look for stubs under both /usr and /usr/local as a fallback on Linux and OS X? If a user wants to manually install some extra stubs that would be available everywhere, they might want to put them under /usr/local. Under OS X my sys.prefix is /Library/Frameworks/Python.framework/Versions/3.5, however.

I agree with @gvanrossum in that the locations should be described in the docs. I'd rather have complete paths instead of talking about sys.prefix or site.USERBASE and similar. I think that it would useful to document at least these cases (and test them):

  • System Python on Linux, OS X and Windows.
  • Virtualenv on Linux, OS X and Windows.
  • Brew on OS X.

If you can't test some of the environments we can try to find somebody who can help.

@tharvik
Copy link
Contributor Author

tharvik commented Jul 22, 2016

@gvanrossum I'll update the docs then, having something similar as PEP 484; I don't have much time this week, I'll try to find a bit of time but I'm not such.
@JukkaL I don't have anything else than Linux, so I'd gladly use the help of someone having the other systems. I used sys.prefix and site.USERBASE because it was the default in the documentation, I'll expand these then (but it will be less portable, as there can be system where prefix is not standard; it is also less readable IMO).

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 22, 2016

@tharvik It's probably okay to use sys.prefix and site.USERBASE, but we may want to hard code some locations in addition to those, depending on the platform. Not very clean but the world is a messy place :-)

@tharvik tharvik force-pushed the add_installed_typehints branch 2 times, most recently from bf61a5e to 96e4d45 Compare July 25, 2016 14:16
@tharvik
Copy link
Contributor Author

tharvik commented Jul 25, 2016

Here goes the updated documentation and the updated default paths, it was tested on Linux system python and Linux virtualenv python.

Mypy will also look in some default install directory to try to find annotations.
For example, for UNIX based, with python 3.4, it will try to find it in
- ``${HOME}/.local/shared/typehints/python3.4/``
- ``/usr/shared/typehints/python3.4/``
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard directory is /usr/share/ (no d). And I think it would be better to put python* before typehints in the path, to mirror /usr/lib/pythonX.Y/. I'd also rather use /usr/lib/python* for this rather than introduce a second python tree under /usr/share (and maybe put it under site-packages?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually based on the example in PEP 484 in chapter store and distribute stub files on how to add it to a setup.py.

...
data_files=[
    (
        'shared/typehints/python{}.{}'.format(*sys.version_info[:2]),
        pathlib.Path(SRC_PATH).glob('**/*.pyi'),
    ),
],
...

The standard directory is /usr/share/ (no d).

From the example, it is indeed 'shared', the '/usr' is because setup.py will install data_files under sys.prefix on system-wide install (based on install info on docs.python.org)

And I think it would be better to put python* before typehints in the path, to mirror /usr/lib/pythonX.Y/.

That's taken from the example, so that would require a PEP change, but I guess it can easily be introduce as it isn't really implemented for now.

I'd also rather use /usr/lib/python* for this rather than introduce a second python tree under /usr/share (and maybe put it under site-packages?).

/usr/lib/python* is for modules, not really for related data; IMO both options are valid, because typehints are closely related to code, but in itself, it isn't necessary for the code to run, thus not going into site-packages. /usr/ is also the standard place to store modules' related data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm referring to the filesystem hierarchy standard: http://www.pathname.com/fhs/pub/fhs-2.3.html#USRSHAREARCHITECTUREINDEPENDENTDATA

If the PEP says /usr/shared, then the PEP should be corrected before this part of it is implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, I see, and that makes sense, it's nicer. Then that would be a design choice coming from @gvanrossum or @JukkaL, and a separated issue IMO.

But I disagreed on 'before this part of it is implemented', we can merge it for now, and update mypy when the path changes are accepted in the PEP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@tharvik tharvik Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to rephrase/intepret what was said, correct me if I'm wrong

  • not having a separated python tree outside of /usr/lib/python*/site-packages
  • at least, using UNIX standard install location (not /usr/shared but /usr/share for example)

I guess, a way to do it would be to change the PEP to not use data_files but rather use package_data, as these are closer to the implementation than having a separated tree.

@bdarnell is that okay for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package_data is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened peps#67 about it, we can move the discussion there.

@gvanrossum gvanrossum changed the title add support for locally installed typehints [review wanted] add support for locally installed typehints Oct 10, 2016
@gvanrossum
Copy link
Member

gvanrossum commented Nov 9, 2016

Hey @tharvik,

It looks like this PR is pretty much stuck. I'm trying to clean up stuck PRs so that the list of PRs is not too long. If you feel like working on this some more, just go ahead and reopen it, or open a new PR -- the corresponding issue will remain open with a reference to this (closed) PR in it, so if someone else feels like tackling this problem they still have this PR as a starting point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants